Conversation
dividedmind
requested review from
apotterri,
dustinbyrne,
kgilpin and
symwell
dividedmind
force-pushed
the
fix/faster-comment-extraction
branch
from
c0a5518 to
f894886
Compare
|
|
||
| # Read file and get last comment before line. | ||
| def self.last_comment(file, line) | ||
| File.open(file) do |f| |
There was a problem hiding this comment.
The other implementation cached file contents. Adding caching here might be only a few lines.
There was a problem hiding this comment.
line is used twice, as an input parameter and do parameter.
There was a problem hiding this comment.
The other implementation cached the whole file per method, which is a bit of a waste of memory. This one caches just the comment.
There was a problem hiding this comment.
I tried to test this with method_source testcases and got
$ ruby --version
ruby 3.0.1p64 (2021-04-05 revision 0fb782ee38) [x86_64-linux]
$ bundle exec rspec spec
1) MethodSource Methods should return a comment for method
Failure/Error:
f.lines.lazy.take(line_number - 1).reverse_each do |line|
break unless (line =~ /^\s*#/) || (line =~ /^\s*$/)
buffer << line.lstrip
end
NoMethodError:
undefined method `lines' for #<File:/home/test/src/method_source/spec/spec_helper.rb (closed)>
Did you mean? lineno
# ./lib/method_source.rb:52:in `block in comment_describing_custom'
# ./lib/method_source.rb:49:in `open'
# ./lib/method_source.rb:49:in `comment_describing_custom'
# ./lib/method_source.rb:44:in `comment_helper'
# ./lib/method_source.rb:143:in `comment'
# ./spec/method_source_spec.rb:69:in `block (3 levels) in <top (required)>'
Maybe f.lines doesn't exist in Ruby 3.0?
Which version of Ruby did you use?
There was a problem hiding this comment.
In ruby 2.6.10, 2.7.2 and 3.0.1 it works with
f.each_line.lazy.take(line_number - 1).reverse_each do |line|
AppMap needs to extract method comments to apply labels. This change makes this process faster. Co-authored-by: symwell <111290954+symwell@users.noreply.github.com>
dividedmind
force-pushed
the
fix/faster-comment-extraction
branch
from
46fe4ad to
f6199a6
Compare
|
Thanks @symwell, good catch! I've merged your changes into my commit, please take a look. |
symwell
left a comment
symwell
left a comment
There was a problem hiding this comment.
Verified locally that it works.
|
🎉 This PR is included in version 0.90.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
4 participants
AppMap needs to extract method comments to apply labels. This change makes this process faster.
Note this is roughly the same change as in #279 but (IMO) a bit cleaner, plus it avoids reading the whole file by using IO#lines iterator and only taking the required number of lines. As such it fixes #276 (in a way).